-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Components/progress bar #1756
Components/progress bar #1756
Conversation
|
@NAsriGhada type compatibility is due to misaligned as for using css classnames rather than tailwind, why not use tailwind utility classes directly? |
@@ -0,0 +1,21 @@ | |||
@import '@radix-ui/colors/black-alpha.css'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why importing radix colors? we only use the colors provided by our custom tailwind config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah just for testing, I will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty good start, just a little bit changes here and there and it's ready to be merged!
please go over my requested changes and let me know here if you have updates or questions.
@@ -0,0 +1,66 @@ | |||
import * as Progress from '@radix-ui/react-progress'; | |||
import React from 'react'; | |||
import { tv } from 'tailwind-variants'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why importing tv but never using it? I suggest removing this import
style={{ | ||
width, | ||
height, | ||
backgroundColor, | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I tried that, but since the colors are dynamic in the progress bar it didn't work as I expected it to be, but I'll try to find a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I tried that, but since the colors are dynamic in the progress bar it didn't work as I expected it to be, but I'll try to find a solution.
Read this and you'll have an idea of how to do it
https://tailwindcss.com/docs/content-configuration#dynamic-class-names
value={progress} | ||
> | ||
<Progress.Indicator | ||
style={indicatorStyle} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interface ProgressProps { | ||
startColor?: string; | ||
endColor?: string; | ||
backgroundColor?: string; | ||
width?: string; | ||
height?: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend you take the component structure from shadcn.
This will ensure you follow the best practices and customize the component however you see fit.
width: `${progress}%`, | ||
}; | ||
|
||
// const progressClass = progress === 100 ? endColor : startColor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this commented line should be deleted
className="transition-width duration-500 ease-in-out" | ||
/> | ||
</Progress.Root> | ||
<p>Progress: {progress}%</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be <span id="loadinglabel">
tag rather than paragraph.
and the Progress.Root
should have this prop
aria-labelledby="loadinglabel"
taken from mdn
startColor: { | ||
control: 'color', | ||
description: 'The starting color of the progress bar.', | ||
}, | ||
endColor: { | ||
control: 'color', | ||
description: 'The color of the progress bar when progress is complete.', | ||
}, | ||
width: { | ||
control: 'text', | ||
description: 'The width of the progress bar.', | ||
}, | ||
height: { | ||
control: 'text', | ||
description: 'The height of the progress bar.', | ||
}, | ||
backgroundColor: { | ||
control: 'color', | ||
description: 'The background color of the progress bar container.', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are auto generated by storybook and you don't have to write them in .stories
files.
To have a description of each prop try adding jsdoc to each typescript prop definition.
example from packages/UI/src/components/Checkbox/Checkbox.tsx
/**
* When true, prevents the user from interacting with the checkbox.
*/
disabled?: RadixCheckbox.CheckboxProps['disabled'];
/**
* The controlled checked state of the checkbox. Must be used in conjunction with onCheckedChange.
*/
checked?: RadixCheckbox.CheckedState & NonNullable<unknown>;
/**
* The checked state of the checkbox when it is initially rendered. Use when you do not need to control its checked state.
*/
defaultChecked?: RadixCheckbox.CheckedState & NonNullable<unknown>;
import ProgressBar from './ProgressBar'; | ||
|
||
const meta: Meta<typeof ProgressBar> = { | ||
title: 'Components/ProgressBar', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this, omitting the title makes storybook read the path of .stories
file and place it in here.
const meta: Meta<typeof ProgressBar> = { | ||
title: 'Components/ProgressBar', | ||
component: ProgressBar, | ||
tags: ['autodocs'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this as docs are always true in our config
startColor, | ||
endColor, | ||
backgroundColor, | ||
width, | ||
height, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these props aren't needed as they're all going to be fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok perfect!
theme: { | ||
control: { | ||
type: 'select', | ||
options: 'light', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when did we decide to include this theme prop?
backgroundColor: 'bg-gray-300', | ||
width: 'w-80', | ||
height: 'h-6', | ||
theme: 'light', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when did we decide to include this theme prop?
/** | ||
* the theme prop controls the appearance based on predefined styles in a themes object. | ||
*/ | ||
theme: 'light'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when did we decide to include this theme prop?
progress, | ||
theme = 'light', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when did we decide to include this theme prop?
backgroundColor, | ||
width, | ||
height, | ||
// backgroundColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why commenting backgroundColor instead of deleting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOOKS AMAZING, you've really nailed this one
I left a tiny request change that can totally be overlooked and a PR that adds to this one here with each commit explaining why it the change was made:
#1788
Please go over the linked PR and let me know if you have questions and if not merge the linked PR then merge this yours after.
thanks a ton Ghada!
className={`${styles.flex} ${styles.flexDirection} ${styles.alignItems} gap-14 rounded-lg p-4`} | ||
> | ||
<div | ||
className={`${styles.flex} ${styles.width} ${styles.flexDirection} ${styles.alignItems} gap-3`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think styles.flex,styles.width, styles.flexDirection , styles.alignItems can be used directly instead
ls there a reason why you made it in an object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah simply for code readability and the DRY principle which made the lines shorter.
progressbar changes
🎉 This PR is included in version 2.17.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
I'm facing an error when using Radix UI components in our Tailwind-configured project. After migrating from JSX I got a different error, attached in the screenshot, which suggests a type incompatibility with the Progress components as far as I understood.
Inline styles were a successful interim solution for the initial Tailwind conflict, but this TypeScript issue persists.